fix(sqleditor): skip read-only columns when saving inserted rows#10015
Open
asheshv wants to merge 1 commit into
Open
fix(sqleditor): skip read-only columns when saving inserted rows#10015asheshv wants to merge 1 commit into
asheshv wants to merge 1 commit into
Conversation
Query Tool result sets can include expression or alias columns (e.g. `first_name || ' ' || last_name AS the_name`) that are not columns of the underlying table. The editability check correctly marked them is_editable=False, but the new-row save flow still sent every column from the row object, so the rendered INSERT referenced the alias and Postgres rejected the row with `column "the_name" does not exist`. Filter on both sides: - get_columns_types now propagates is_editable into the column metadata stored on the session. - save_changed_data drops non-editable keys from the added-row data before rendering insert.sql. - ResultSet.jsx omits cells whose column has can_edit=false from the added-row payload sent to the server. Adds a regression scenario in test_save_changed_data exercising the exact concatenation-alias query from the issue report. Fixes pgadmin-org#9939
WalkthroughThe PR fixes Query Editor failures when saving new rows in result sets containing non-editable columns (aliases, expressions). The fix filters such columns on both frontend and backend: metadata propagation adds ChangesNon-editable column filtering in Query Editor saves
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/pgadmin/tools/sqleditor/utils/save_changed_data.py`:
- Around line 121-129: The filtering of incoming row data currently treats
unknown column keys as editable because the predicate uses columns_info.get(k,
{}).get('is_editable', True); change the default to False so unknown keys are
rejected: update the comprehension that builds data (the dict comprehension that
references columns_info and 'is_editable') to use .get('is_editable', False) so
only known editable columns are retained before INSERT rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce5ec567-c303-4937-8f91-14fa9791425d
📒 Files selected for processing (4)
web/pgadmin/tools/sqleditor/static/js/components/sections/ResultSet.jsxweb/pgadmin/tools/sqleditor/utils/get_column_types.pyweb/pgadmin/tools/sqleditor/utils/save_changed_data.pyweb/pgadmin/tools/sqleditor/utils/tests/test_save_changed_data.py
Comment on lines
+121
to
+129
| # Drop any column the client included that isn't a real | ||
| # editable column of the underlying table (e.g. | ||
| # `first_name || ' ' || last_name as the_name`). Without | ||
| # this guard the rendered INSERT references a non-existent | ||
| # column and Postgres rejects the row. Issue #9939. | ||
| data = { | ||
| k: v for k, v in data.items() | ||
| if columns_info.get(k, {}).get('is_editable', True) | ||
| } |
There was a problem hiding this comment.
Reject unknown client keys in added-row filtering.
The current predicate defaults unknown keys to editable (True), so non-schema keys can still pass through and reach INSERT rendering. That undermines the defense-in-depth this block is adding.
Proposed fix
- data = {
- k: v for k, v in data.items()
- if columns_info.get(k, {}).get('is_editable', True)
- }
+ data = {
+ k: v for k, v in data.items()
+ if k in columns_info and
+ columns_info[k].get('is_editable', True)
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/pgadmin/tools/sqleditor/utils/save_changed_data.py` around lines 121 -
129, The filtering of incoming row data currently treats unknown column keys as
editable because the predicate uses columns_info.get(k, {}).get('is_editable',
True); change the default to False so unknown keys are rejected: update the
comprehension that builds data (the dict comprehension that references
columns_info and 'is_editable') to use .get('is_editable', False) so only known
editable columns are retained before INSERT rendering.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #9939.
Query Tool result sets can include expression/alias columns (e.g.
first_name || ' ' || last_name AS the_name) that don't exist on the underlying table. The editability detection inis_query_resultset_updatable.pyalready marks themis_editable=False, so the column header correctly shows the lock icon. But when a user adds a new row, the frontend populated every column key on the new row object and shipped the whole thing to/sqleditor/save.save_changed_dataaccepted those keys verbatim, and the rendered INSERT referenced the alias — Postgres replied withcolumn "the_name" does not exist in relation "some_table".Filter on both sides:
get_columns_typesnow propagatesis_editableinto the column metadata stored on the session (is_editable=Truedefaulted for View Data Tool, where columns are always real table columns).save_changed_datadrops non-editable keys from the added-rowdatabefore renderinginsert.sql. Defense-in-depth: protects against stale/buggy clients.ResultSet.jsx::triggerSaveDataomits cells whose column hascan_edit === falsefrom the added-row payload.Test plan
python regression/runtests.py --pkg tools.sqleditor.utils.tests.test_save_changed_data— 13/13 pass, including the newTestSaveAddedRowSkipsNonEditableColumnscenario that runs the issue's exactfirst_name || ' ' || last_name AS the_nameSELECT and verifies the INSERT succeeds.python regression/runtests.py --pkg tools.sqleditor.utils.tests.test_is_query_resultset_updatable— 10/10 pass (3 OID-only scenarios skipped on PG12+, as before).python regression/runtests.py --pkg tools.sqleditor.utils.tests.test_save_changed_uuid_data— 3/3 pass (the other consumer of the changed helpers).make check-pep8,yarn run linterclean.Summary by CodeRabbit
Release Notes